Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

Chore: Refactor the codebase (fixes #220) #261

Merged
merged 1 commit into from
May 6, 2017
Merged

Chore: Refactor the codebase (fixes #220) #261

merged 1 commit into from
May 6, 2017

Conversation

JamesHenry
Copy link
Member

@JamesHenry JamesHenry commented May 6, 2017

This is major refactoring of the structure of the core codebase.

There are no changes to any existing test cases.

Summary of changes:

  • Drop Node 0.12 support, now >=4 to align with eslint/eslint
  • Remove now redundant object-assign dependency, and replace its usage with native Object.assign
  • Remove features.js and token-translator.js - they were never used
  • Use same lint configuration as eslint/eslint
  • Upgrade all dependencies to latest
  • Update ast-node-types.js to include all known converted ESTreeNode types
  • Add errorOnUnknownASTType parser option to throw if a generated ESTreeNode type is not found in ast-node-types.js (this is disabled by default, but enabled in tests)
  • Separate utility functions out into a new file, node-utils.js, to make the codebase much easier to reason about, especially w.r.t. which helpers mutate the current node, and which are pure functions
  • Move all ts.* helper functions into abstractions within node-utils.js so that it is clear what our dependencies on TypeScript actually are
  • Add brand new general utilities such as findFirstMatchingChild and findFirstMatchingAncestor which take in predicate functions, and are built upon by other utility functions. These also allow me to remove some ts.* dependencies from the codebase
  • Move core convert() function into its own file, convert.js, and increase what is explicitly passed into it rather than just closing over all outer scopes. Again, this greatly helps with clarity and focused responsibility
  • Add block scoping to more complex switch cases in convert.js, taking advantage of let and const for variable declarations. No more battling to come up with unique variable names within a 1000+ line function!

@JamesHenry JamesHenry requested a review from soda0289 May 6, 2017 18:27
@eslintbot
Copy link

LGTM

Copy link
Member

@soda0289 soda0289 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow this looks great! I like how you broke util functions into its own files. We could probably start to do that with the node converts as well at some point.

Merging this in will cause all current PR to need rebase. Not sure what order will work best.

.travis.yml Outdated
@@ -1,7 +1,6 @@
language: node_js
sudo: false
node_js:
- "0.12"
- 4
- 6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add Node v7 as well?

/**
* Expose the enum of possible TSNode `kind`s.
*/
exports.SyntaxKind = SyntaxKind;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe we should move the exports to the bottom of the file and do it all in one object. That way it might be easier to know what a file exports. If this was typescript we could just export the function or variables but I don't care too much.

@eslintbot
Copy link

LGTM

@JamesHenry
Copy link
Member Author

@soda0289 Yes fair point about the readability of exports, thanks! I have addressed that, and dialed up the linting strictness to fully match eslint/eslint (caught a lot of additional formatting points in test and Makefiles etc).

I updated the travis to run on 4, 5, 6 and 7, again to match eslint/eslint.

With regards to merging, I would love to get this in ASAP, and then continue working on more fixes. Are you happy to close out your old PRs and start branching off from this work?

@soda0289
Copy link
Member

soda0289 commented May 6, 2017

Yea I can do that.

@JamesHenry
Copy link
Member Author

Thanks, sorry that this is awkward one - long overdue! I think I will get Pajn's breaking change one in first, then rebase this and merge it.

Then we could do an actual release I think, because all the breaking changes will be in.

@soda0289
Copy link
Member

soda0289 commented May 6, 2017

Could we merge this into:
#247 Prolly won't need a rebase.

I would like to get the comment scanner fixed before we release. Could you review that PR?

@eslintbot
Copy link

LGTM

@JamesHenry
Copy link
Member Author

Both of those are in now, thanks!

@soda0289
Copy link
Member

soda0289 commented May 6, 2017

Lets merge this then. I'll rebase the comment scanner code off of this change.

@JamesHenry JamesHenry merged commit f836bb9 into master May 6, 2017
@JamesHenry JamesHenry deleted the refactor branch May 6, 2017 22:35
@JamesHenry
Copy link
Member Author

🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants